-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Initial refactor #1
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some comments and requests for a few changes.
@@ -0,0 +1,2 @@ | |||
[settings] | |||
profile=black |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this do?
3. Extract the GPS data from the video. | ||
4. Load the video, audio, and GPS data into the repository. | ||
|
||
Args: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not consistent with the actual arguments of the function
|
||
def process_video( | ||
video_path: str, | ||
audio_path: str, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this the desired output path?
import job_config as jc | ||
|
||
|
||
# TODO: Maybe this should stream to file instead of loading everything into memory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
slight disadvantage is that the separate loading would not be part of the same transaction.
if "Altitude (ft)" in df.columns: | ||
df["Altitude (m)"] = df["Altitude (ft)"] * 0.3048 | ||
else: | ||
df["Altitude (m)"] = -1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the Netherlands for example altitude -1 is not uncommon.
can we make somehow ensure that missing data would be inserted as a DB NULL?
But NULL would also be more explicitly indicating missing data than some number.
DROP TABLE dbo.video; | ||
END | ||
|
||
CREATE TABLE dbo.video ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to add a "video_create_date datetime" column. this data is, I think, contained in the JSON, but would be nice to have as explicit metadata. There may be some other fields we might also want to add as explicit columns, but this one is I think useful. for example to make absolute timestamps from relative times in text_segment.
# Extract the audio from the video | ||
with utils.LogTime(logger, "Getting audio/metadata from video"): | ||
vp.extract_audio(video_path, audio_path, logger=logger) | ||
video_metadata = vp.get_metadata(video_path, logger=logger) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see DDL.sql: would be good I think to extract video_create_date from the metadata and make that an explicit field. so should be set here.
Btw, video_create_data iso just create_date as latter pattern is often used for when a row was added to the table.
gps_filename VARCHAR(1024) NOT NULL, | ||
checksum VARCHAR(64) NOT NULL, | ||
metadata VARCHAR(MAX) NOT NULL, -- JSON | ||
CONSTRAINT PK_video PRIMARY KEY(id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually would be good ALSO to have "last_update_date datetime default getdate()" here and all other tables so we see when rows were added.
|
||
# https://stackoverflow.com/a/39215961/2691018 | ||
class StreamToLogger: | ||
"""This class redirects write calss to a logger.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type: calss iso calls
assert os.path.exists(ffmpeg_binaries) | ||
os.environ["PATH"] = f"{ffmpeg_binaries}:" + os.environ["PATH"] | ||
|
||
base_path = "/home/idies/workspace/nist_ai/data/video/Tanya" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reminder that we just agreed we will put all videos under
/home/idies/workspace/nist_ai/data/video
and we'll have subfolders GoPro/, TrackAddict/, and Other/ without further hierearchy for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more comments
# - altitude_m: float | ||
# - speed_kmh: float | ||
with utils.LogTime(logger, "Parsing gps file"): | ||
gps_data = gp.parse_gps_file( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if I am not mistaken parse_gps_file throws an exception if the file does not exist which is not caught here. so does that mean there is no way to load videos without GPD file?
Only refactors backend code. UI needs to be refactored too.
Able to convert and save a single file. Example seen in demo.py
I would like to refactor a bit more in the final product. I want to put everything in a package along with the UI code.